Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fiery Explosion Effect and Condition #2779

Merged
merged 11 commits into from
Jul 20, 2020
Merged

Fiery Explosion Effect and Condition #2779

merged 11 commits into from
Jul 20, 2020

Conversation

APickledWalrus
Copy link
Member

@APickledWalrus APickledWalrus commented Jan 19, 2020 •
edited
Loading

Description

Continuing the current theme of explosion-related PR's, this PR adds a new effect and condition. It allows you to set/check if an explosive entity's explosion will be fiery. It also allows you to set this in the ExplosionPrimeEvent. The syntax could maybe be better for the event part, so I'm open to any suggestions.
EDIT: I will fix the test for this soon.

Target Minecraft Versions: Any
Requirements: None
Related Issues: None

@Wealthyturtle Wealthyturtle added the feature Pull request adding a new feature. label Jan 19, 2020
@APickledWalrus APickledWalrus changed the title Fiery Explosion Stuff Fiery Explosion Effect and Condition Jan 19, 2020
@APickledWalrus
Copy link
Member Author

Fixed the condition which also fixed the test.

bensku
bensku previously approved these changes Feb 29, 2020
View reviewed changes
@APickledWalrus
Copy link
Member Author

Did you mean to add that commit?

@bensku
Copy link
Member

bensku commented Feb 29, 2020

Yeah, and I think this is ready to be merged. Just waiting for one more approval.

ShaneBeee
ShaneBeee suggested changes Mar 5, 2020
View reviewed changes
Copy link
Contributor

@ShaneBeee ShaneBeee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, just 1 small thing I noticed.

src/main/java/ch/njol/skript/conditions/CondExplodesWithFire.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffExplodeWithFire.java Outdated Show resolved Hide resolved
@APickledWalrus APickledWalrus added this to the 2.5 milestone Jul 10, 2020
@APickledWalrus
Copy link
Member Author

I'm not sure if I really like the syntax for entities in this.
make %entities% [(1¦not)] cause (a fiery|an incendiary explosion)
To me, that implies that the entities are going to cause a fiery explosion when that effect is ran.
The documentation could also include an example on how to use this with entities.

@FranKusmiruk
Copy link
Member

Forgot to mention, the class names may as well be changed to CondIncendiary and EffIncendiary and so the test script.

@APickledWalrus
Copy link
Member Author

APickledWalrus commented Jul 11, 2020 •
edited
Loading

Syntax should be better now 😃

Edit: I always forget to update the test D:

@ShaneBeee ShaneBeee dismissed stale reviews from FranKusmiruk and Pikachu920 July 20, 2020 19:51

This is all good to go

@ShaneBeee ShaneBeee merged commit 6df393e into SkriptLang:master Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShaneBeee ShaneBeee ShaneBeee approved these changes

@bensku bensku bensku left review comments

@Pikachu920 Pikachu920 Pikachu920 left review comments

@FranKusmiruk FranKusmiruk FranKusmiruk left review comments

Assignees
No one assigned
Labels
feature Pull request adding a new feature.
Projects
None yet
Milestone
2.5
Development

Successfully merging this pull request may close these issues.
6 participants
@APickledWalrus @bensku @FranKusmiruk @ShaneBeee @Pikachu920 @Wealthyturtle